apps/meteor/client/providers/UserProvider/UserProvider.tsx (2)

147-149: 💤 Low value

Consider removing implementation comment per coding guidelines.

The comment explains the internal behavior of getUserPreference and why the settings subscription is needed. While this is architecturally useful context, the coding guideline for TypeScript files asks to avoid code comments in implementation.

Consider whether this comment adds sufficient value to warrant an exception, or if the behavior could be documented elsewhere (e.g., in PR description, commit message, or the getUserPreference function itself).

As per coding guidelines: "Avoid code comments in the implementation" for **/*.{ts,tsx,js}.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/meteor/client/providers/UserProvider/UserProvider.tsx` around lines 147
- 149, Remove the implementation comment explaining getUserPreference's fallback
and the settings.watch subscription from UserProvider.tsx; either delete it or
relocate that rationale to the getUserPreference function's JSDoc or the
PR/commit message. Specifically, update the comment near the settings.watch
invocation in the UserProvider component and ensure any necessary documentation
lives on getUserPreference (its function comment) or external documentation so
the implementation file follows the "avoid code comments in implementation"
guideline.

---

141-158: 💤 Low value

Consider using .toString() for explicit ObjectId conversion.

The code uses String(key) to convert ObjectId | string to string (lines 150, 156). While String() coercion works, using String(key) on mixed types is less explicit than checking the type and calling .toString() when needed, or using a type guard.

📝 More explicit conversion approach

-			const subscribe = (onStoreChange: () => void): (() => void) => {
+			const keyStr = String(key);
+			const subscribe = (onStoreChange: () => void): (() => void) => {
 				const unsubUsers = Users.use.subscribe(onStoreChange);
 				// getUserPreference falls back to settings.watch(`Accounts_Default_User_Preferences_${key}`)
 				// when the user record has no override. Subscribe to that specific
 				// setting key so admin-side default changes still propagate.
-				const unsubSettings = settings.observe(`Accounts_Default_User_Preferences_${String(key)}`, onStoreChange);
+				const unsubSettings = settings.observe(`Accounts_Default_User_Preferences_${keyStr}`, onStoreChange);
 				return () => {
 					unsubUsers();
 					unsubSettings();
 				};
 			};
-			const getSnapshot = (): T | undefined => getUserPreference(userId, String(key), defaultValue);
+			const getSnapshot = (): T | undefined => getUserPreference(userId, keyStr, defaultValue);
 			return [subscribe, getSnapshot];

This hoists the conversion to clarify that both subscribe and getSnapshot use the same string representation.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/meteor/client/providers/UserProvider/UserProvider.tsx` around lines 141
- 158, The queryPreference helper currently uses String(key) in both subscribe
and getSnapshot; change this to compute a single stringKey by checking the type
of key and calling key.toString() when key is an ObjectId (or otherwise using
the existing string), then use stringKey in Users.use.subscribe/settings.observe
and in getUserPreference(userId, stringKey, defaultValue) so the conversion is
explicit and consistent across subscribe and getSnapshot (refer to
queryPreference, subscribe, getSnapshot, getUserPreference, settings.observe,
and Users.use.subscribe).